Skip to content

C#: Prepend enclosing method in local function TRAP labels#2061

Merged
calumgrant merged 1 commit into
github:masterfrom
hvitved:csharp/local-function-label
Oct 1, 2019
Merged

C#: Prepend enclosing method in local function TRAP labels#2061
calumgrant merged 1 commit into
github:masterfrom
hvitved:csharp/local-function-label

Conversation

@hvitved

@hvitved hvitved commented Sep 30, 2019

Copy link
Copy Markdown
Contributor

This PR fixes the underlying issue of #2018. The issue was closed, because the false positive was no longer there following the latest dist upgrade. However, it turns out that the bug was merely hidden (by #1851), and then revealed again with #2054.

The underlying problem is that if a file gets compiled twice, into assemblies A.dll and B.dll, then all methods in the file will exist in two copies (by design), as the method labels are prefixed with A and B. However, local functions do not contain the assembly prefix, so -- because of the TRAP stack -- only the firstly imported TRAP file will get a copy of the local function. The solution is to prepend the ID of the enclosing callable when generating TRAP labels for local functions, in order to also get a copy per assembly.

@hvitved hvitved added the C# label Sep 30, 2019
@hvitved hvitved requested a review from calumgrant September 30, 2019 17:17
@hvitved hvitved requested a review from a team as a code owner September 30, 2019 17:17
@hvitved hvitved force-pushed the csharp/local-function-label branch from 322ddde to 413926f Compare October 1, 2019 08:42

@calumgrant calumgrant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calumgrant calumgrant merged commit b4da63b into github:master Oct 1, 2019
@hvitved hvitved deleted the csharp/local-function-label branch October 1, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants